Integrating class symbol support #16

andyleejordan · 2023-01-11T18:18:13Z

WIP: Attempting to integrate PowerShell#1886 with the changes previously made in PowerShell#1917.

andyleejordan · 2023-01-11T18:40:10Z

@SeeminglyScience with the integration pretty much all the changes you had to make to the unit tests I reverted. Can you explain if any of those were intentional (beyond just "making them pass")?

andyleejordan

My own questions for you both.

src/PowerShellEditorServices/Services/Symbols/ReferenceTable.cs

src/PowerShellEditorServices/Services/Symbols/SymbolsService.cs

test/PowerShellEditorServices.Test/Language/SymbolsServiceTests.cs

test/PowerShellEditorServices.Test.Shared/References/FunctionReference.ps1

andyleejordan · 2023-01-11T19:12:57Z

Hm ok so the sorting isn't quite right. Debugging FindsReferencesOnTypeConstraint and while all four references are found, they're out of order because the one used as the param type constraint has a StartOffset of 0.

fflaten · 2023-01-15T16:18:30Z

Currently shows variable-assignments for every parameter and re-assignments inside functions/methods in outline + workspace search. Feels messy compared to stable.

Maybe stick with current behavior for now? If so, it might require either of these:

Sticking with FindSymbolsVisitor for documentsymbols and workspacesymbols handlers.
Add another flag or bool in SymbolReference to control visibility in the handlers
Include a Parent-backlink in the SymbolReference we can filter on in handler (to keep only root-level). This is easier to achieve now with a single reference visitor and is probably required to support DocumentSymbol.children anyway, so I'd prefer this.

test/PowerShellEditorServices.Test/Language/SymbolsServiceTests.cs

fflaten · 2023-01-15T18:40:53Z

src/PowerShellEditorServices/Services/Symbols/SymbolsService.cs

+            if (file.References.TryGetReferences(foundSymbol.SymbolName, out ConcurrentBag<SymbolReference> references))
+            {
+                return references;
+            }


Suggested change

if (file.References.TryGetReferences(foundSymbol.SymbolName, out ConcurrentBag<SymbolReference> references))

{

return references;

}

if (file.References.TryGetReferences(foundSymbol.SymbolName, out ConcurrentBag<SymbolReference> references))

{

return references.Where((r) =>

foundSymbol.SymbolType == r.SymbolType ||

(foundSymbol.SymbolType is SymbolType.Class or SymbolType.Enum or SymbolType.Type

&& r.SymbolType is SymbolType.Class or SymbolType.Enum or SymbolType.Type)

);

}

Filter on symbol type. Will currently match ex. a function and enum/class/type with same name.

andyleejordan · 2023-01-18T21:45:48Z

Include a Parent-backlink in the SymbolReference we can filter on in handler (to keep only root-level). This is easier to achieve now with a single reference visitor and is probably required to support DocumentSymbol.children anyway, so I'd prefer this.

Same, that's what I was thinking.

fflaten · 2023-01-20T23:18:21Z

src/PowerShellEditorServices/Services/Symbols/ReferenceTable.cs

+            return AstVisitAction.Continue;
+        }
+
+        public override AstVisitAction VisitInvokeMemberExpression(InvokeMemberExpressionAst methodCallAst)


Which scenario is this for? I don't think you'll be able to match it to the correct method to count it as a reference.

So it turns out it works quite well! By bucketing all the overloads (of the same name) together, and returning a set of possible definitions, VS Code just does what it needs so you can do:

class SuperClass : BaseClass { [string]MyClassMethod([string]$param1, $param2, [int]$param3) { $this.SomePropWithDefault = 'something happend' $param1 return 'finished' } [string] MyClassMethod([MyEnum]$param1) { return 'hello world' } [string]MyClassMethod() { return 'hello world' } } $o = [SuperClass]::new() $o.MyClassMethod()

and find definition on MyClassMethod() shows the three potential matches:

No, it's not perfect, but it's a whole lot better than nothing!

True, go to definition makes sense. 👍 You could also filter on argument count, but argument types won't work.

To make it complete, should ::new() return the constructors? Because that's a reserved method when static (InvokeMemberExpressionAst.Static == true), right?

That's a good idea!

Bumps [Microsoft.PowerShell.SDK](https://github.com/PowerShell/PowerShell) from 7.3.1 to 7.3.2. - [Release notes](https://github.com/PowerShell/PowerShell/releases) - [Commits](PowerShell/PowerShell@v7.3.1...v7.3.2) --- updated-dependencies: - dependency-name: Microsoft.PowerShell.SDK dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Name was wrong.

To actually be an inclusive check of the given point being between the two boundary points.

Space between name and subsequent parenthesis.

Let it be known that Patrick bets $500 that no one notices this. And to be fair, searching the whole workspace now works great.

And delete "find details" for non-commands (as they all get their display strings set on creation now).

The visitor exists but it's not called, probably a module-import issue.

Co-authored-by: Patrick Meinecke <[email protected]>

So that we rely on the dictionary to sort our symbols into the equivalent types instead of having to perform a filter.

I think there's a cleaner way to do this, but this works.

Against the full name and not just the identifier, since it's what's displayed in the search menu. Now you can search methods by their parameters' names.

Should have been just `IndexOf`, and also use `+` instead of `string.Concat()` for simplicity and speed.

andyleejordan force-pushed the andschwa/better-symbols branch from a4f8b8b to 77494c4 Compare January 11, 2023 18:36

andyleejordan commented Jan 11, 2023

View reviewed changes

andyleejordan force-pushed the andschwa/better-symbols branch from 261360c to 70bcd04 Compare January 12, 2023 19:36

fflaten reviewed Jan 15, 2023

View reviewed changes

test/PowerShellEditorServices.Test/Language/SymbolsServiceTests.cs Outdated Show resolved Hide resolved

fflaten reviewed Jan 15, 2023

View reviewed changes

andyleejordan force-pushed the andschwa/better-symbols branch 2 times, most recently from 82ec11f to 604d6a7 Compare January 20, 2023 22:12

fflaten reviewed Jan 20, 2023

View reviewed changes

fflaten mentioned this pull request Jan 21, 2023

Integrating class symbol support PowerShell/PowerShellEditorServices#1984

Merged

dependabot bot and others added 17 commits January 24, 2023 21:39

add symbols for classes

7270bed

remove code used for supporting PSv3 and v4

69a66bb

support hover

e3845e8

add type reference symboltype

b1187b9

support references and definitions

7cf22f0

add codelens for class and enum

13adfc3

skip function ast visit for FunctionMemberAst

f2be365

drop reference-support for ConfigurationDefinition

1d1b5cc

update document symbols test

50e8a6c

refactor and cleanup

4c3a4ab

fix Contains extension method for same line extent

ad58bcd

add EnumMember, signature and fix detection

12b1366

update symbols test with name position

884fecb

check type for property and function members

109dbb0

add symbolsservice tests

e776984

add codelens tests

4be7724

andyleejordan added 15 commits January 26, 2023 17:35

Fix find variable and function references tests

2592af9

Fix FindsOccurrencesOnParameter test

e2d4e25

Fix FindsReferencesOnCommandWithAlias test

bd76db3

Fix a bunch of class symbol tests

04345dd

Fix FindsPropertyDefinition test

2729e83

Name was wrong.

Fix property symbols tests

b634dfa

Fix ScriptRegion.ContainsPosition

e990670

To actually be an inclusive check of the given point being between the two boundary points.

Fix display string for function definitions

e407453

Space between name and subsequent parenthesis.

Fix find enum member tests

2f93517

Add symbol type equivalencies test

fe29524

Remove deprecated dot-source reference tests (and data)

f69694c

Let it be known that Patrick bets $500 that no one notices this. And to be fair, searching the whole workspace now works great.

Fix FindsDetailsForBuiltInCommand test

aa6bfc7

And delete "find details" for non-commands (as they all get their display strings set on creation now).

Fix the rest of the symbols service tests

1829093

Run FindsDetailsForBuiltInCommand only on macOS

6a21c57

Skip FindsSymbolsInDSCFile as DSC symbols don't yet work

6d8b0ba

The visitor exists but it's not called, probably a module-import issue.

andyleejordan force-pushed the andschwa/better-symbols branch from be3e601 to 6d8b0ba Compare January 27, 2023 20:41

andyleejordan and others added 12 commits January 27, 2023 18:33

Mark some methods internal

fd04c34

Co-authored-by: Patrick Meinecke <[email protected]>

Rename SymbolReference fields

3010065

Replace SymbolTypeMatches logic with prefixed identifiers

0135baa

So that we rely on the dictionary to sort our symbols into the equivalent types instead of having to perform a filter.

Strip qualifier from variable name in symbol identifier

7da6f9d

Handle symbol prefix identifier in CommandHelpers

4a90f28

I think there's a cleaner way to do this, but this works.

More leniently filter workspace symbols

3b08189

Against the full name and not just the identifier, since it's what's displayed in the search menu. Now you can search methods by their parameters' names.

Map symbol types to LSP better

6d2d5a7

Restore public ScriptRegion Create(...) method

4c1538b

Fix bugs in VisitorUtils around LastIndexOf

9e12670

Should have been just `IndexOf`, and also use `+` instead of `string.Concat()` for simplicity and speed.

Distinguish properties/enums from variables

0739fc1

Distinguish constructors/methods from functions

4296553

Remove unused code from Extensions.cs

6208b53

andyleejordan closed this Feb 2, 2023

andyleejordan deleted the andschwa/better-symbols branch February 2, 2023 20:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrating class symbol support #16

Integrating class symbol support #16

andyleejordan commented Jan 11, 2023

andyleejordan commented Jan 11, 2023

andyleejordan left a comment

andyleejordan commented Jan 11, 2023

fflaten commented Jan 15, 2023 •

edited

Loading

fflaten Jan 15, 2023 •

edited

Loading

andyleejordan commented Jan 18, 2023

fflaten Jan 20, 2023

andyleejordan Jan 21, 2023

fflaten Jan 21, 2023

fflaten Jan 21, 2023

andyleejordan Jan 23, 2023

-            if (file.References.TryGetReferences(foundSymbol.SymbolName, out ConcurrentBag<SymbolReference> references))
-            {
-                return references;
-            }
+            if (file.References.TryGetReferences(foundSymbol.SymbolName, out ConcurrentBag<SymbolReference> references))
+            {
+return references.Where((r) =>
+                    foundSymbol.SymbolType == r.SymbolType ||
+                    (foundSymbol.SymbolType is SymbolType.Class or SymbolType.Enum or SymbolType.Type
+                        && r.SymbolType is SymbolType.Class or SymbolType.Enum or SymbolType.Type)
+                );
+            }

Integrating class symbol support #16

Integrating class symbol support #16

Conversation

andyleejordan commented Jan 11, 2023

andyleejordan commented Jan 11, 2023

andyleejordan left a comment

Choose a reason for hiding this comment

andyleejordan commented Jan 11, 2023

fflaten commented Jan 15, 2023 • edited Loading

fflaten Jan 15, 2023 • edited Loading

Choose a reason for hiding this comment

andyleejordan commented Jan 18, 2023

fflaten Jan 20, 2023

Choose a reason for hiding this comment

andyleejordan Jan 21, 2023

Choose a reason for hiding this comment

fflaten Jan 21, 2023

Choose a reason for hiding this comment

fflaten Jan 21, 2023

Choose a reason for hiding this comment

andyleejordan Jan 23, 2023

Choose a reason for hiding this comment

fflaten commented Jan 15, 2023 •

edited

Loading

fflaten Jan 15, 2023 •

edited

Loading